[FIX] [RPC] Align getrawtransaction with Bitcoin Core#985
[FIX] [RPC] Align getrawtransaction with Bitcoin Core#985moisesPompilio wants to merge 4 commits into
Conversation
044b3b7 to
6099cc1
Compare
|
This PR is in draft because it depends on #821. That PR introduces typos for spell checking, allowing more flexibility when using randomly generated addresses without triggering spelling errors |
9547e4a to
c9ac410
Compare
c9ac410 to
22e29a2
Compare
|
22e29a2 I rebased and converted the added integration test to the pytest format. |
| #[command(name = "gettransaction")] | ||
| GetTransaction { txid: Txid, verbose: Option<bool> }, | ||
| #[command(name = "getrawtransaction")] | ||
| GetRawTransaction { txid: Txid, verbose: Option<u32> }, |
There was a problem hiding this comment.
I dont think that Bitcoin Core will appear with 4 294 967 295 levels of verbosity
There was a problem hiding this comment.
I agree, but u32 is the default for numeric verbosity that we use.
There was a problem hiding this comment.
u32 occupies too much space for something simple as a verbosity level.
Also: #1076
There was a problem hiding this comment.
I replaced the u32 with a u8.
| match verbosity { | ||
| 0 => serde_json::to_value(serialize_hex(&tx.tx)) | ||
| .map_err(|e| JsonRpcError::Decode(e.to_string())), | ||
| 1 => serde_json::to_value(self.make_raw_transaction(tx)) | ||
| .map_err(|e| JsonRpcError::Decode(e.to_string())), | ||
| _ => return Err(JsonRpcError::InvalidVerbosityLevel), | ||
| } | ||
| } |
There was a problem hiding this comment.
Return a structured type instead of a generic
|
|
||
| /// The information returned by a get_raw_tx | ||
| #[derive(Deserialize, Serialize)] | ||
| pub struct RawTxJson { |
There was a problem hiding this comment.
Because the GetRawTransaction of corepc is not good, the coinbase field of the input is missing, and in addition, old fields are present.
22e29a2 to
d67326e
Compare
|
nit: the bang ( |
| if verbosity > 1 { | ||
| return Err(JsonRpcError::InvalidVerbosityLevel); | ||
| } |
There was a problem hiding this comment.
nit: Since there's a wildcard inside the match below, this check is useless.
There was a problem hiding this comment.
Yeah, I didn’t want the verbosity check to be after getting the transaction, but it doesn’t really matter, since it would only save a few milliseconds in the process. So I removed this if; the validation is in the match now.
| pub enum GetRawTransactionRes { | ||
| Zero(String), | ||
|
|
||
| One(Box<RawTxJson>), | ||
| } |
There was a problem hiding this comment.
nit:
| pub enum GetRawTransactionRes { | |
| Zero(String), | |
| One(Box<RawTxJson>), | |
| } | |
| pub enum GetRawTransactionRes { | |
| Zero(String), | |
| One(Box<RawTxJson>), | |
| } |
Also, you Box-ed it because it is too big?
There was a problem hiding this comment.
Yes, we are adopting Box for the response enums because the verbosity objects tend to be very large.
| } | ||
|
|
||
| /// The information returned by a get_raw_tx | ||
| #[derive(Debug, Deserialize, Serialize)] |
There was a problem hiding this comment.
| #[derive(Debug, Deserialize, Serialize)] | |
| #[derive(Debug, Deserialize, Serialize)] | |
| /// The information returned by a get_raw_tx |
d67326e to
7331f65
Compare
|
Needs rebase |
…osity to numeric type The RPC client was incorrectly calling 'gettransaction' (which doesn't exist on the server) instead of 'getrawtransaction'. Additionally, the verbosity parameter handling was inconsistent. While the method signature correctly accepted Option<u32>, it needed better standardization to match Bitcoin Core's RPC specification.
…format This commit aligns the getrawtransaction RPC response with Bitcoin Core's format and behavior. The following changes were made: RawTx struct: - Made blockhash, confirmations, blocktime, and time optional fields They are now only serialized when present, avoiding null values - Fixed txid encoding that was previously inverted TxOut struct: - Changed value from u64 (sats) to f64 (BTC) for proper denomination - Made address field optional, only serialized when script can be converted to a valid address TxIn struct: - Added coinbase field that only appears in coinbase transactions, containing the hex-encoded script from scriptSig - Coinbase inputs have: coinbase, sequence, witness (optional) - Regular inputs have: txid, vout, script_sig, sequence, witness (optional) Script handling: - Improved ASM (assembly) format conversion to strictly follow Bitcoin Core rules and formatting standards TxOut types: - Updated to include all script types that Bitcoin Core supports Note: The `desc` field was not implemented due to complex matching rules. This will be added in a future PR.
Add comprehensive integration test comparing Floresta's `getrawtransaction` RPC output with Bitcoin Core for verbosity levels 0 and 1, covering: - Regular transactions (`P2PKH`, `P2WPKH`, `P2SH`, `P2TR types`) - Coinbase transactions - Input/output field validation - Optional field handling New BaseRPC methods: - `get_raw_transaction()` New BitcoinRPC methods: - `create_wallet()` - `get_new_address()` - `generate_block_to_wallet()` - `send_to_address()`
7331f65 to
d0b8750
Compare
|
d0b8750 I’ve rebased |
Description and Notes
Brings Floresta’s
getrawtransactionRPC closer to Bitcoin Core compatibility.Fixes the RPC method call so the client correctly uses
getrawtransactioninstead of the incorrectgettransaction, and standardizesverbosityhandling to match Bitcoin Core’s numeric behavior.The response format was aligned with Bitcoin Core as well:
RawTxfields such asblockhash,confirmations,blocktime, andtimeare now optional and only serialized when present.txidencoding was corrected.TxOut.valuenow usesf64in BTC instead ofu64in sats, matching Bitcoin Core’s denomination.TxOut.addressis now optional and only included when the script can be converted into a valid address.TxInnow handles coinbase inputs properly, including thecoinbasefield only when applicable.TxOutscript types were expanded to cover the script types supported by Bitcoin Core.Note: the
descfield was intentionally left out, since matching Bitcoin Core’s descriptor logic requires additional investigation and a more complex implementation. That can be handled in a future PR.How to verify the changes you have done?
Compare the output of
getrawtransactionbetween Floresta and Bitcoin Core for bothverbosity = 0andverbosity = 1.The integration test added here already covers this behavior and is the best reference for validation.
P2PKH,P2WPKH,P2SH, andP2TR.